Conversation
|
Release the wolves! I'm prepared to be eaten alive for this PR. :) |
|
You'd better be ready to talk this over on Wednesday's call. :)
|
There was a problem hiding this comment.
The main question I have about this is about the difference between this SpawnHelper and the public API (C, and Python) we already provide.
- It seems that some functions are just duplicated here. I flagged a couple.
- Some of these functions could potentially benefit the public API and should be moved there
I guess one argument one could make would be that the public API is C, not C++, but this is a non-argument. We could just have the functionality implemented in a C++ method, and the C one would just reinterpret_cast/cast and forward to the C++ one.
If you look at runtime.h, we have a C++ and C function for the callbacks
C++
ENERGYPLUSLIB_API void callbackBeginNewEnvironment(EnergyPlusState state, std::function<void(EnergyPlusState)> f);C
extern "C" {
ENERGYPLUSLIB_API void callbackBeginNewEnvironment(EnergyPlusState state, void (*f)(EnergyPlusState));
}Here is a dumb demo: https://compiler-explorer.com/z/8951PYh6o
| [[nodiscard]] int ActuatorHandle(EnergyPlus::EnergyPlusData &state, | ||
| const std::string &componentType, | ||
| const std::string &controlType, | ||
| const std::string &componentName); |
There was a problem hiding this comment.
| const std::string &controlType, | ||
| const std::string &componentName); | ||
|
|
||
| void SetActuatorValue(EnergyPlus::EnergyPlusData &state, int handle, Real64 value); |
There was a problem hiding this comment.
|
|
||
| void SetActuatorValue(EnergyPlus::EnergyPlusData &state, int handle, Real64 value); | ||
|
|
||
| void ResetActuator(EnergyPlus::EnergyPlusData &state, int handle); |
There was a problem hiding this comment.
|
I would like to see just an extension of the API to cover the additional functions that Spawn needs, rather than creating a special interface. |
|
@kbenne @Myoldmopar it has been 28 days since this pull request was last updated. |
3 similar comments
|
@kbenne @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@kbenne @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@kbenne @Myoldmopar it has been 28 days since this pull request was last updated. |
6 participants
Pull request overview
Description of the purpose of this PR
Pull Request Author
Reviewer